-
Notifications
You must be signed in to change notification settings - Fork 1.6k
🌱 generalize tools Dockerfiles for arm64, ppc64le linux builds #1992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 generalize tools Dockerfiles for arm64, ppc64le linux builds #1992
Conversation
/cc @DirectXMan12 @Adirio |
- 'GOOS=${_GOOS}' | ||
- 'GOARCH=${_GOARCH}' | ||
images: ["gcr.io/kubebuilder/thirdparty-${_GOOS}:1.20.2"] | ||
- name: "gcr.io/kubebuilder/thirdparty-${_GOOS}-${_GOARCH}:${_KUBERNETES_VERSION}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we keep something around that cross-tags this to the old thirdparty-${OS}
format too, so we don't confuse people? (this is a question, not a suggestion -- I can't quite convince myself either way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this image intended to be pulled/used outside of this build system? If so then I don’t think the old format should be kept around because it is now ambiguous.
EDIT: actually for v1.19 I think we can make an exception because released kubebuilder projects already use the old format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: nevermind, we are distributing tarballs, not docker images. Docker is just used as the build environment.
args: [ | ||
"-h", "Content-Type:application/gzip", | ||
"cp", | ||
"/workspace/kubebuilder-tools-${_KUBERNETES_VERSION}-${_GOOS}-${_GOARCH}.tar.gz", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here.
In either case, will need to update the go.kubebuilder.io
shortlinks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tarball name format hasn't changed, and if it has it shouldn't have since it already includes os and arch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea is generally fine, couple of questions inline
0dc0a18
to
4f928d1
Compare
build/cloudbuild_tools.yaml: update with new Dockerfile args, prettify README.md: add new arches Signed-off-by: Eric Stroczynski <[email protected]>
4f928d1
to
8f5b2ab
Compare
- `_GOARCH=arm64 _GOOS=linux` | ||
- `_GOARCH=ppc64le _GOOS=linux` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do this need to be listed somewhere else? I mean you will need to configure this list of os/arch somewhere (maybe not in code though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think @DirectXMan12 needs to update these vars in gcloud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Adirio, estroz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Note: this PR also downgrades k8s version to v1.19.2 so arm64 and ppc64le builds occur for a v1.19 version. I will post a follow-up PR to upgrade the version back to v1.20.2 after this is merged.
Closes #966
Signed-off-by: Eric Stroczynski [email protected]